-
-
Notifications
You must be signed in to change notification settings - Fork 21.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Redesign Quick Open #56772
Redesign Quick Open #56772
Conversation
For long file paths, ellipsis ( |
a5c717f
to
8e6bf4b
Compare
0decbbd
to
9595a74
Compare
3a881b6
to
302288f
Compare
This PR is ready for review! There are a few gui/theme issues I can't figure out, but these aren't blocking issues and fixing it could increase the scope too much (for me at least), so I think it's better if they are be fixed later.
I also looked into making the dialog a singleton. It makes some sense given that we allocate all these nodes for the results on startup. However, that only works if only one object subscribed to the quick open signal at any moment. |
The current looks of this dialog goes very against the overall visual style of the Godot editor:
|
What's the status of this? The remarks from the previous comment should be addressed. Also the PR needs rebase. |
a00f209
to
c00370c
Compare
The new dialog is functional, although there are still some things I don't like in the code. I've already mentioned a few styling issues, and I don't know how to fix them. Any help with theming/UI issues, or code review would be appreciated. |
To me, it's kind of clear that Godot's design might be lacking, which is why I redesigned this dialog in the first place. I designed it precisely without caring about what 'style' Godot has, only caring about what a Quick Open dialog needs.
I'm okay with making the results part darker, but I think the bottom part should still be even darker. Here is what it looks like (ignore the white dialog border, I think it happened after rebasing, maybe a theme update?).
No, that would not be better. It would no longer be a simple search bar, and even on it's own, that place is not a better location. There is free space at the bottom, and putting it there also doesn't break the simple design of search bar + search results.
There is a good reason, as I've explained in the post above.
No.
I don't think it works well with Godot's separate windows, but I have seen this style work in other software. It's up for debate whether we want to keep this or not.
So? |
If you think Godot's overall UI design needs changes that's completely fine, but this sort of thing needs to be done as a whole and with proper discussion on what said changes should be (the proposals repository would be the perfect place for that). Having a section of the editor look and act differently from the rest just adds confusion to the user, who expects things to behave consistently across the editor. |
Top down, bottom up. Both is possible, but I have no interest in getting bogged down in discussions over what 'design rules' Godot should follow. I think it's more effective to try to design a small isolated component, and maybe others can take that further. Actually, I think that because of this top down approach, Godot's style (which was supposed to have gotten a complete overhaul in 4.0 btw) has remained so stagnant, even when there is so much room for improvement. |
4fa91dd
to
9cb4f5f
Compare
5575be8
to
080e81e
Compare
Some updates
|
c0f1b1d
to
da5d417
Compare
- Updated list view with thumbnails, and separate file name. - Added a grid view which has larger icons. - Added toggle to filter out files from addons. - Store history for each opened resource type. New Editor settings for Quick Open: - Startup display mode (grid or list): - Determined by the requested resource type. - Whatever was last used. - Toggle to filter out files from addons (for persistence). Notes - The dialog is now created once in EditorNode, and globally available for other components. - A fixed number of result scenes are instantiated, and reused based on query. - Drop support for multiselect.
da5d417
to
06791e1
Compare
Well I already approved it; just gave a quick test now and it's still fine. We can still improve it after merging. |
I know @samsface and @a-johnston have been working on improving the fuzzy matching for the Quick Open dialog in #82200. Does this PR also impact the fuzzy matching, or only improves the UI side of things? CC @MajorMcDoom too. |
From my testing, it allows non-tokenized matching, like before #88660 (so now both are possible). idk if it affects results, they look same/similar as before. |
Alright, if contributors interested in the search/fuzzy matching behavior are fine with merging this as a first step, and then doing further work on top of it, this should be good to go. |
In my view, the work I did is outdated from a UX needs perspective and thus if anything supersedes it, it's a good thing. So I'm totally good with this. |
Thanks @stijn-h for this great overhaul, almost 3 years in the making! 🎉 |
Yeah when @samsface linked this I think we both expected to just rebase on top whenever it merged. The match highlighting logic might be a bit funky but ultimately the change sets are pretty separate. BTW very cool changes! I really like the look of the new popup. |
Closes godotengine/godot-proposals#3788
Features: